We mentioned that any database operation requiring a lock context could be refactored using the functor pattern, but I think this case might be an exception—even after applying the refactor.

The functor isn’t particularly useful here since we don’t have the block ID until we start executing the deferred database operations.

I went ahead and refactored it anyway to illustrate my point, but in this case, it doesn’t provide any performance benefits over the original version and only adds unnecessary complexity.

Thoughts?

Instead of storing each individual guarantee, we pass in all the guarantees to be stored together, so that instead of creating N functors, we create only one functor to insert and index N guarantees.

In this PR, I'm using guarantees as an example to apply the functor pattern and raise the discussion. I'd like to get feedbacks first and settle on the functor pattern before applying this to the storing operation of the rest of payloads , such as seals, results etc.

Why creating this struct?

Because we would like to insert N guarantees with one functor, instead of N functors. And we need to bundle all the data before passing to the functor.

Why placing this struct here?

It might not be the best place, but at least it's close to where it is being used (InsertAndIndexGuarantees). I'm open for ideas for a better place for this struct.

Because we would like to insert N guarantees with one functor, instead of N functors.

Do we have any evidence that using N functors would have a non-negligible performance impact? I think it is likely to have an extremely small impact in practice, and don't think it is worth the complexity of optimizing for here.

Also, the need to pre-compute the guarantee IDs prior to passing to the functor is downstream of a lack of ID caching. General ID caching is a problem we will probably want to solve relatively soon, which would remove the need for this (we would just pass the Guarantee object around and call ID() as much as we want without per-call performance penalty). Again, unless there is evidence that we really need the optimization for this case, I suggest we opt for the simpler implementation.

Regarding the err message, the functors like Overwriting and InsertingWIthMismatchCheck is too general that doesn't have the context. So I used WrapError to include more context.

This is redundant, because InsertAndIndexResultApproval will check the lock

Replaced by functors

Wrapping error for inserting multiple records is a bit challenging, I can't find a cleaner way to provide context for each individual write. So ended up just wrapping the process.

This function is unused, but only for reference for now.

I initially created this function by combining InsertGuarantee and IndexGuarantee, and refactored with the functor pattern. But later I decided to use InsertAndIndexGuarantees, see comments there for my motivation.

I kept this one here for reference as it's easier to understand, will clean up after the discussion of functor pattern is settled.

I prefer the term "upsert": it matches the non-functor function with the same semantics, and better captures that we are either inserting or updating (we are only overwriting if something has already been written).

I don't think separating this out from lock proof checking is a good idea. This function will not behave as expected if the user does not include lock checking separately. Currently there is not documentation of this critical requirement of the user of this function. We could add documentation, but I think it would be better to logically couple lock proof checking with corresponding read/write checks.

I like the idea, but I thinking adding too many layers of indirection will make it harder to write and verify correct use of locks. I think it would be better to couple each of these generic lock-requiring utility functions with HoldingLock.

There are lots of boilerplates, like the error to return, it allows us to work with higher abstraction.

We could also make a InsertingWithExistenceCheckAndLockCheck to bind with the lock checking, but IMO it's unnecessary.

So we are able to simplify the functor operation as:

func InsertAndIndexGuarantees(guaranteesWithID []*CollectionGuaranteeWithID) Functor {
    ...
	return BindFunctors(
		CheckHoldLockFunctor(storage.LockInsertBlock),
		UpsertMulFunctor(guaranteeIDKeys, guarantees),
		InsertFunctorMulWithMismatchCheck(collectionIDKeys, guaranteeIDs),
    )
}

We could also make a InsertingWithExistenceCheckAndLockCheck to bind with the lock checking

I don't think we should have a separate function that also binds the lock check. I think InsertingWithExistenceCheck should remain as it is, except that it also does the lock check. The reason is that InsertingWithExistenceCheck must never be called on its own, it always must be called strictly after a lock check.

Currently these two things which must always happen together are separate in the API, and the fact that they must be manually called together every time for correct usage is not documented. I'm suggesting that we modify the API so that the only way to call InsertingWithExistenceCheck is to simultaneously define the required lock check.

What about keeping both:

func InsertAndIndexGuarantees(guaranteesWithID []*CollectionGuaranteeWithID) Functor {
    ...
	return BindFunctors(
+		// keep this lock check here to make the functor composition flexible
		CheckHoldLockFunctor(storage.LockInsertBlock), 
		UpsertMulFunctor(guaranteeIDKeys, guarantees),
-		InsertFunctorMulWithMismatchCheck(collectionIDKeys, guaranteeIDs),
+		InsertFunctorMulWithMismatchCheck(
+			storage.LockInsertBlock, // check if this lock is held before running the insert
+			collectionIDKeys, guaranteeIDs),
    )
}

I have the same worry here about separating generic logic that must be lock-protected from the logic checking the lock, as mentioned above.

Because we would like to insert N guarantees with one functor, instead of N functors.

Do we have any evidence that using N functors would have a non-negligible performance impact? I think it is likely to have an extremely small impact in practice, and don't think it is worth the complexity of optimizing for here.

Also, the need to pre-compute the guarantee IDs prior to passing to the functor is downstream of a lack of ID caching. General ID caching is a problem we will probably want to solve relatively soon, which would remove the need for this (we would just pass the Guarantee object around and call ID() as much as we want without per-call performance penalty). Again, unless there is evidence that we really need the optimization for this case, I suggest we opt for the simpler implementation.

General suggestion on naming

I think explicitly using the Functor terminology in the name of functor-returning storage functions would be clearer than using the present tense.

• Present tense vs imperative doesn't really communicate "this will be done in the future" to me
• The "functor" version is usually named very similarly to the "non-functor" version (eg. Index vs Indexing) -- I feel it would be easy to gloss over and miss the distinction

I'm suggestion as a naming convention: $(name you would have used had it not been a functor) + "Functor"